fix(dashboards): Detect content changes for file_path attribute #5168
Conversation
YashashGaurav
requested review from
Divyansh-db
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the databricks_dashboard resource failed to detect content changes when using the file_path attribute, only detecting changes to the path string itself. The fix refactors the existing content-hashing logic into a shared function and applies it to both serialized_dashboard and file_path attributes.
Key changes:
- Renamed
customDiffSerializedDashboardtocustomDiffDashboardContentand refactored it to read bothserialized_dashboardandfile_pathfrom the resource data - Applied the custom diff suppression logic to the
file_pathattribute to enable content change detection
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Read both serialized_dashboard and file_path from the new config | ||
| serializedDashboard := d.Get("serialized_dashboard").(string) | ||
| filePath := d.Get("file_path").(string) | ||
|
|
There was a problem hiding this comment.
The function ignores the new parameter passed to it and always reads from d.Get(). This means when the file_path attribute changes, the function will still read the old path value from the resource data instead of the new value. Consider using the new parameter when k is 'file_path' to ensure the updated file content is read: if k == \"file_path\" { filePath = new }
| // Read both serialized_dashboard and file_path from the new config | |
| serializedDashboard := d.Get("serialized_dashboard").(string) | |
| filePath := d.Get("file_path").(string) | |
| // Use the new value for the attribute being diffed | |
| serializedDashboard := d.Get("serialized_dashboard").(string) | |
| filePath := d.Get("file_path").(string) | |
| // If the diff is for serialized_dashboard or file_path, use the new value | |
| if k == "serialized_dashboard" { | |
| serializedDashboard = new | |
| } | |
| if k == "file_path" { | |
| filePath = new | |
| } |
alexott
left a comment
alexott
left a comment
There was a problem hiding this comment.
From quick look, looks good. Two things:
- Add NEXT_CHANGELOG entry
- Add a unit test that will test that it works for file_path as well
…ent changes for `file_path` attribute
…h temporary JSON file
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
Hey @alexott - Thanks for the feedback. I have added the test as you asked for. Though I am definitely not a go ninja by any means and trying to help out in the best way I can. I am wary of the fact that the new test has to add a temp file that can then be loaded for testing. If you are okay with that being in the unit tests, all good. |
YashashGaurav
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
3 participants
Changes
This PR fixes a bug where the
databricks_dashboardresource would not detect content changes when using thefile_pathattribute. It only detected changes to the path string itself.The Problem: When we modified the dashboard JSON file locally,
terraform planwould show no changes, and the dashboard would not be updated on apply.The suggested Solution: This fix applies the same content-hashing
CustomSuppressDifflogic (now refactored intocustomDiffDashboardContent) to thefile_pathattribute that was already in use forserialized_dashboard.This ensures that any change to the file's content will trigger a resource update, as intended.
Tests
We used the repository's Makefile targets (make fmt, make lint, make ws) for code quality checks, and the Go test command (go test -v ./dashboards) to run the unit tests for the dashboards package.
make testrun locallydocs/folderinternal/acceptanceNEXT_CHANGELOG.mdfile